Fix #2839: initialize punc_res before conditional to prevent UnboundLocalError#2840
Fix #2839: initialize punc_res before conditional to prevent UnboundLocalError#2840Lidang-Jiang wants to merge 2 commits intomodelscope:mainfrom
Conversation
…t UnboundLocalError When punc_model is None or not provided, punc_res was only assigned inside the `if self.punc_model is not None:` block. Downstream code paths (punc_segment speaker diarization and sentence_timestamp) accessed punc_res unconditionally, causing UnboundLocalError. Changes: - Initialize punc_res = None before the conditional block - Add punc_res is None guard in punc_segment path with error log - Add punc_res is None guard in sentence_timestamp path with warning log - Add unit tests covering punc_model=None, empty punc_model, and normal flow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an UnboundLocalError in the inference_with_vad method by ensuring punc_res is properly initialized and checked before use when punc_model is None. The changes also include code formatting improvements and a new test suite to verify the fix. Feedback suggests refining the docstring in the new test file to accurately reflect that only the punc_model=None case is tested, as empty strings are not covered and may lead to different initialization issues.
tests/test_punc_model_none.py
Outdated
| @@ -0,0 +1,119 @@ | |||
| """Tests for issue #2839: punc_model=None or empty string should not cause UnboundLocalError.""" | |||
There was a problem hiding this comment.
The docstring states that these tests cover punc_model=None or an empty string. However, the tests only cover the None case. An empty string for punc_model would likely cause a different failure mode during model initialization, not the UnboundLocalError this PR aims to fix. To avoid confusion, please update the docstring to only mention the punc_model=None case.
| """Tests for issue #2839: punc_model=None or empty string should not cause UnboundLocalError.""" | |
| """Tests for issue #2839: punc_model=None should not cause UnboundLocalError.""" |
There was a problem hiding this comment.
Good catch -- updated the module docstring to only mention punc_model=None, since the tests don't cover the empty string case. Fixed in 7bd02d8.
The tests only cover the None case, not the empty string case. Update the module docstring to match actual test coverage. Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
1 participant
Summary
Fix
UnboundLocalErrorwhenpunc_modelisNoneor not provided inAutoModel.generate()with VAD enabled.Root cause: In
inference_with_vad(),punc_resis only assigned inside theif self.punc_model is not None:block, but downstream code paths (punc_segmentspeaker diarization andsentence_timestamp) access it unconditionally.Fix:
punc_res = Nonebefore the conditional blockpunc_res is Noneguards with appropriate log messages in the two affected code pathsBefore (UnboundLocalError on unpatched code)
After (all tests pass)
Test plan
test_punc_model_none_basic— basic ASR withpunc_model=Nonereturns correct texttest_sentence_timestamp_with_punc_model_none—sentence_timestamp=Truewithpunc_model=Nonereturns emptysentence_infoinstead of crashingtest_punc_model_with_value_still_works— normal flow withpunc_modelprovided still produces punctuated text🤖 Generated with Claude Code